Windows support #951

mataton · 2024-04-05T23:28:56Z

A continuation of PR #949, this PR adds support for Windows.

All issues revolve around two things:

File path separators differing between operating systems. / vs. \
Temporary file handling differing between operating systems.

The first issue is solved with a straightforward fix using os.join() and os.path.sep.

The second issue is more complex. In Windows, the name of a NamedTemporaryFile object cannot be used to open the file a second time, while the named temporary file is still open. See this SO, and the Python docs. The strategy is to use the delete=False parameter to prevent the temporary file from being deleted when it is closed, and then manually delete the file when the test is done using it.

wasade

Thanks, @mataton!!! This is super helpful, and glad to see the changes seem relatively minor! The primary comments are capturing the tmp filename for unlinking in tearDown

wasade · 2024-04-10T16:19:46Z

biom/tests/test_cli/test_add_metadata.py

            fh.write(biom1)
            fh.flush()
            self.biom_table1 = biom.load_table(fh.name)
+        os.unlink(fh.name)


Could self.temporary_fh_name be set within the scope of the context manager, and move the os.unlink to tearDown? The reason for the former is the current approach relies on scope creep which works but borders on surprising behavior. The latter is important as, if a test fails, cleanup will only reliably occur if the unlink is performed by tearDown.

wasade · 2024-04-10T16:20:06Z

biom/tests/test_cli/test_summarize_table.py

            fh.write(biom1)
            fh.flush()
            self.biom1 = load_table(fh.name)
+        os.unlink(fh.name)


Same here per comment above

wasade · 2024-04-10T16:21:37Z

biom/tests/test_cli/test_table_converter.py

            fh.write(biom1)
            fh.flush()
            self.biom_table1 = load_table(fh.name)
+            fh.close()


Was it necessary to close here but not in the other TestCases? And same here re unlinking. If it's viable to just unlink within the context manager than that may be easier than the tearDown

If you close the file before unlinking within the context manager, then it works. Otherwise you get a PermissionError at the unlinking step. Looking at the code now, I think the idea of setting self.temporary_fh_name in the context manager and putting the unlink in tearDown is the best solution.

wasade · 2024-04-10T16:21:47Z

biom/tests/test_parse.py

            save_table(t, tmpfile.name)
            obs = load_table(tmpfile.name)
            self.assertEqual(obs, t)
+        os.unlink(tmpfile.name)


wasade · 2024-04-10T16:22:13Z

biom/tests/test_table.py

            h5 = h5py.File(tmpfile.name, 'w')
            t.to_hdf5(h5, 'tests')
            h5.close()

            h5 = h5py.File(tmpfile.name, 'r')
            obs = Table.from_hdf5(h5)
+            h5.close()
+        os.unlink(tmpfile.name)


These tests are creating the temporary files outside of a setUp/tearDown context, so the solution involving putting the unlink in tearDown won't work here. The reason the closes are necessary within the context manager is that without them unlink will throw PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\me\\AppData\\Local\\Temp\\tmpg4pk3ink' . My understanding is that the context manager will close tmpfile when it is finished, but it won't close h5. Exactly why this is the case I'm unsure of.

I think this is the case because I set delete=False during the file creation, so a file will not be deleted when it is closed. But it seems on windows that file handles must be closed before the file may be deleted.

So this solution works because it closes all file handles, including tmpfile and h5, and then deletes the file using unlink. I'm not sure of a better solution for this particular case and the other similar cases.

Issue is edge case if test excepts prior to unlink. A common strategy for this type of stuff:

def setUp(self): self.temp_files = [] def tearDown(self): for f in self.temp_files: os.unlink(f) def test_some_test(self): ... self.temp_files.append(blah) ...

wasade · 2024-04-10T16:22:18Z

biom/tests/test_table.py

            h5 = h5py.File(tmpfile.name, 'w')
            t.to_hdf5(h5, 'tests', creation_date=current)
            h5.close()

            h5 = h5py.File(tmpfile.name, 'r')
            obs = Table.from_hdf5(h5)
            self.assertEqual(obs.create_date, current)
+            h5.close()
+        os.unlink(tmpfile.name)


wasade · 2024-04-10T16:22:25Z

biom/tests/test_table.py

            h5 = h5py.File(tmpfile.name, 'w')
            t.to_hdf5(h5, 'tests')
            h5.close()
+        os.unlink(tmpfile.name)


wasade · 2024-04-10T16:22:52Z

biom/tests/test_table.py

            h5 = h5py.File(tmpfile.name, 'w')
            t.to_hdf5(h5, 'tests')
            h5.close()
+        os.unlink(tmpfile.name)


...I'll stop flagging on these

wasade · 2024-04-10T16:23:45Z

biom/tests/test_table.py

-                                                              'generated-by',
-                                                              'creation-date',
-                                                              'shape', 'nnz']))
+                                                            'format-url',


The change in indent here looks incorrect as it should be aligned with the [?

mataton · 2024-04-11T19:12:12Z

@wasade Essentially it looks like there are two scenarios. One where all the tests in whichever test.py file require the temporary files. In this case, the setUp/tearDown solution I feel is best. This is true in test_add_metadata.py, test_summarize_table.py, and test_table_converter.py.

The other scenario is that not all of the tests within the test.py require the temporary files, so using setUp/tearDown framework doesn't make sense here. The current solution in those files I think might be the most appropriate, considering my previous comments.

wasade · 2024-04-13T22:29:20Z

.github/workflows/release.yml

+      - name: Build wheels (py ${{ matrix.pyver }}) Windows
+        if: matrix.os == 'windows-latest'
+        env:
+          CIBW_ARCHS_WINDOWS: "amd64 win32 arm64"


Is that 32-bit windows? If so, does the build actually work?

...this repository does not need to support 32-bit architectures

I believe it is 32 bit windows and that it works, although @qiyunzhu wrote this section of the code so he may know more than me.

wasade · 2024-04-13T22:32:05Z

biom/tests/test_table.py

            h5 = h5py.File(tmpfile.name, 'w')
            t.to_hdf5(h5, 'tests')
            h5.close()

            h5 = h5py.File(tmpfile.name, 'r')
            obs = Table.from_hdf5(h5)
+            h5.close()
+        os.unlink(tmpfile.name)


Issue is edge case if test excepts prior to unlink. A common strategy for this type of stuff:

def setUp(self): self.temp_files = [] def tearDown(self): for f in self.temp_files: os.unlink(f) def test_some_test(self): ... self.temp_files.append(blah) ...

wasade · 2024-04-13T22:33:41Z

Thanks, @mataton. I still think we do want unlink in tearDown as that ensures it runs even in the event of an exception, does that make sense? It's relatively minor here though as these are still going into TMPDIR

mataton · 2024-04-15T17:19:28Z

@wasade Your suggested changes make sense, and there was actually already a to_remove = [] in the setUp functions that I didn't notice previously, which I used. Just had to add the proper tearDown to a couple files. It should be good now.

wasade · 2024-04-15T17:28:04Z

Will merge once aarch64 completes

wasade · 2024-04-15T17:28:39Z

...thank you @mataton, this is super helpful!!! One last request, could you add a changelog note?

mataton · 2024-04-15T17:41:29Z

Happy to contribute!

ChangeLog.md

qiyunzhu and others added 6 commits April 4, 2024 09:08

supporting Windows and Python 3.12

8e87369

Fix FileNotFoundErrors to support windows

fc112b8

Merge branch 'qiyun_windows' into windows

1a116cd

Use dirname over rsplit

e656012

Fix temp file opening issues

8ae53dd

Fix other temp file issues

4b3adc9

wasade mentioned this pull request Apr 10, 2024

WIP Supporting Windows and Python 3.12 #949

Closed

wasade requested changes Apr 10, 2024

View reviewed changes

Fix linting and put unlink in tearDown

bdbc4db

mataton changed the title ~~WIP: Windows support~~ Windows support Apr 11, 2024

mataton added 2 commits April 11, 2024 15:45

Testing workflow failure of python3.7 on windows

b0654ba

Remove test line for python3.7

07236ff

wasade reviewed Apr 13, 2024

View reviewed changes

mataton added 2 commits April 15, 2024 09:45

Merge branch 'master' into windows

e38dae5

Utilize setUp and tearDown for removing tmp files

563eda1

wasade approved these changes Apr 15, 2024

View reviewed changes

Update changelog

00a532e

wasade reviewed Apr 15, 2024

View reviewed changes

ChangeLog.md Outdated Show resolved Hide resolved

Update ChangeLog.md

bf11bfd

wasade merged commit 738d240 into biocore:master Apr 15, 2024
2 checks passed

mataton mentioned this pull request May 30, 2024

Windows support for ProteinEmbedding scikit-bio/scikit-bio#2032

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows support #951

Windows support #951

mataton commented Apr 5, 2024 •

edited

Loading

wasade left a comment

wasade Apr 10, 2024

wasade Apr 10, 2024

wasade Apr 10, 2024

mataton Apr 11, 2024

wasade Apr 10, 2024

wasade Apr 10, 2024

mataton Apr 11, 2024

mataton Apr 11, 2024

mataton Apr 11, 2024

wasade Apr 13, 2024

wasade Apr 10, 2024

wasade Apr 10, 2024

wasade Apr 10, 2024

wasade Apr 10, 2024

wasade Apr 10, 2024

mataton commented Apr 11, 2024

wasade Apr 13, 2024

wasade Apr 13, 2024

mataton Apr 15, 2024

wasade Apr 13, 2024

wasade commented Apr 13, 2024

mataton commented Apr 15, 2024 •

edited

Loading

wasade commented Apr 15, 2024

wasade commented Apr 15, 2024

mataton commented Apr 15, 2024

Windows support #951

Windows support #951

Conversation

mataton commented Apr 5, 2024 • edited Loading

wasade left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mataton commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wasade commented Apr 13, 2024

mataton commented Apr 15, 2024 • edited Loading

wasade commented Apr 15, 2024

wasade commented Apr 15, 2024

mataton commented Apr 15, 2024

mataton commented Apr 5, 2024 •

edited

Loading

mataton commented Apr 15, 2024 •

edited

Loading